Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add module for log4cats support for debugging purposes #1896

Merged
merged 4 commits into from
Jul 19, 2023

Conversation

sideeffffect
Copy link
Contributor

@sideeffffect sideeffffect commented Jul 18, 2023

Creates the Log4CatsDebuggingLogHandler class

|
| ${s.linesIterator.dropWhile(_.trim.isEmpty).mkString("\n ")}
|
| arguments = [${a.mkString(", ")}]
Copy link
Collaborator

@jatcwang jatcwang Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think logging arguments would be a security violation in many applications. Perhaps we can make this an option (default false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@jatcwang jatcwang Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jdkLogHandler is not for production use (as written in the scaladoc). Although I think we can update that scaladoc too to provide additional warning that it prints arguments

Copy link
Contributor Author

@sideeffffect sideeffffect Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class would still be very valuable for debugging purposes in non-production environments.
Indeed it is true that it could log sensitive data.

EDIT: I added a scaladoc to make the purpose clear.

""".stripMargin)

case ProcessingFailure(s, a, l, e1, e2, t) =>
logger.warn(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be an error too? (e.g. error in Put/Get definition)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed to me not as severe like ExecFailure, and giving it a different priority also give more power to tune what (and how) is logged.

But if you'd still prefer to have both as errors, no problem, just let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it more, perhaps both these failure cases should be warn level because

  • If the failure isn't intentional, their code is going to fail anyway and their execution will fail with an Exception anwyay.
  • If the (Execution) failure is intentional, then we don't really want to log at error level to avoid triggering alerts.

WDYT? I don't really mind

@sideeffffect sideeffffect changed the title Add module for log4cats support Add module for log4cats support for debugging purposes Jul 19, 2023
@jatcwang jatcwang merged commit 1d4b19b into typelevel:main Jul 19, 2023
@mergify mergify bot added the log4cats label Jul 19, 2023
@sideeffffect sideeffffect deleted the log4cats branch July 19, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants